Minor refactors

Andrew Cantino 10 years ago
parent
commit
556c45e000

+ 3 - 3
app/concerns/oauthable.rb

@@ -11,11 +11,11 @@ module Oauthable
11 11
     true
12 12
   end
13 13
 
14
-  def valid_services(current_user)
14
+  def valid_services_for(user)
15 15
     if valid_oauth_providers == :all
16
-      current_user.available_services
16
+      user.available_services
17 17
     else
18
-      current_user.available_services.where(provider: valid_oauth_providers)
18
+      user.available_services.where(provider: valid_oauth_providers)
19 19
     end
20 20
   end
21 21
 

+ 2 - 2
app/concerns/twitter_concern.rb

@@ -25,11 +25,11 @@ module TwitterConcern
25 25
   end
26 26
 
27 27
   def twitter_oauth_token
28
-    self.service.token
28
+    service.token
29 29
   end
30 30
 
31 31
   def twitter_oauth_token_secret
32
-    self.service.secret
32
+    service.secret
33 33
   end
34 34
 
35 35
   def twitter

+ 0 - 1
app/controllers/services_controller.rb

@@ -1,5 +1,4 @@
1 1
 class ServicesController < ApplicationController
2
-
3 2
   def index
4 3
     @services = current_user.services.page(params[:page])
5 4
 

+ 1 - 1
app/models/agent.rb

@@ -44,7 +44,7 @@ class Agent < ActiveRecord::Base
44 44
   after_save :possibly_update_event_expirations
45 45
 
46 46
   belongs_to :user, :inverse_of => :agents
47
-  belongs_to :service
47
+  belongs_to :service, :inverse_of => :agents
48 48
   has_many :events, -> { order("events.id desc") }, :dependent => :delete_all, :inverse_of => :agent
49 49
   has_one  :most_recent_event, :inverse_of => :agent, :class_name => "Event", :order => "events.id desc"
50 50
   has_many :logs,  -> { order("agent_logs.id desc") }, :dependent => :delete_all, :inverse_of => :agent, :class_name => "AgentLog"

+ 3 - 3
app/models/agents/basecamp_agent.rb

@@ -59,7 +59,7 @@ module Agents
59 59
     end
60 60
 
61 61
     def check
62
-      self.service.prepare_request
62
+      service.prepare_request
63 63
       reponse = HTTParty.get request_url, request_options.merge(query_parameters)
64 64
       memory[:last_run] = Time.now.utc.iso8601
65 65
       if last_check_at != nil
@@ -72,11 +72,11 @@ module Agents
72 72
 
73 73
   private
74 74
     def request_url
75
-      "https://basecamp.com/#{URI.encode(self.service.options[:user_id].to_s)}/api/v1/projects/#{URI.encode(interpolated[:project_id].to_s)}/events.json"
75
+      "https://basecamp.com/#{URI.encode(service.options[:user_id].to_s)}/api/v1/projects/#{URI.encode(interpolated[:project_id].to_s)}/events.json"
76 76
     end
77 77
 
78 78
     def request_options
79
-      {:headers => {"User-Agent" => "Huginn (https://github.com/cantino/huginn)", "Authorization" => "Bearer \"#{self.service.token}\""}}
79
+      {:headers => {"User-Agent" => "Huginn (https://github.com/cantino/huginn)", "Authorization" => "Bearer \"#{service.token}\""}}
80 80
     end
81 81
 
82 82
     def query_parameters

+ 42 - 34
app/models/service.rb

@@ -1,17 +1,22 @@
1 1
 class Service < ActiveRecord::Base
2
+  PROVIDER_TO_ENV_MAP = {'37signals' => 'THIRTY_SEVEN_SIGNALS'}
3
+
2 4
   attr_accessible :provider, :name, :token, :secret, :refresh_token, :expires_at, :global, :options
3 5
 
4 6
   serialize :options, Hash
5 7
 
6
-  belongs_to :user
7
-  has_many :agents
8
+  belongs_to :user, :inverse_of => :services
9
+  has_many :agents, :inverse_of => :service
8 10
 
9 11
   validates_presence_of :user_id, :provider, :name, :token
10 12
 
11 13
   before_destroy :disable_agents
12 14
 
15
+  scope :available_to_user, lambda { |user| where("services.user_id = ? or services.global = true", user.id) }
16
+  scope :by_name, lambda { |dir = 'desc'| order("services.name #{dir}") }
17
+
13 18
   def disable_agents
14
-    self.agents.each do |agent|
19
+    agents.each do |agent|
15 20
       agent.service_id = nil
16 21
       agent.disabled = true
17 22
       agent.save!(validate: false)
@@ -24,52 +29,55 @@ class Service < ActiveRecord::Base
24 29
   end
25 30
 
26 31
   def prepare_request
27
-    if self.expires_at && Time.now > self.expires_at
28
-      self.refresh_token!
32
+    if expires_at && Time.now > expires_at
33
+      refresh_token!
29 34
     end
30 35
   end
31 36
 
32 37
   def refresh_token!
33 38
     response = HTTParty.post(endpoint, query: {
34 39
                   type:          'refresh',
35
-                  client_id:     ENV["#{provider_to_env}_OAUTH_KEY"],
36
-                  client_secret: ENV["#{provider_to_env}_OAUTH_SECRET"],
37
-                  refresh_token: self.refresh_token
40
+                  client_id:     oauth_key,
41
+                  client_secret: oauth_secret,
42
+                  refresh_token: refresh_token
38 43
     })
39 44
     data = JSON.parse(response.body)
40
-    self.update(expires_at: Time.now + data['expires_in'], token: data['access_token'], refresh_token: data['refresh_token'].presence || self.refresh_token)
41
-  end
42
-
43
-  def self.initialize_or_update_via_omniauth(omniauth)
44
-    case omniauth['provider']
45
-    when 'twitter'
46
-      find_or_initialize_by(provider: omniauth['provider'], name: omniauth['info']['nickname']).tap do |service|
47
-        service.assign_attributes(token: omniauth['credentials']['token'], secret: omniauth['credentials']['secret'])
48
-      end
49
-    when 'github'
50
-      find_or_initialize_by(provider: omniauth['provider'], name: omniauth['info']['nickname']).tap do |service|
51
-        service.assign_attributes(token: omniauth['credentials']['token'])
52
-      end
53
-    when '37signals'
54
-      find_or_initialize_by(provider: omniauth['provider'], name: omniauth['info']['name']).tap do |service|
55
-        service.assign_attributes(token: omniauth['credentials']['token'],
56
-                                  refresh_token: omniauth['credentials']['refresh_token'],
57
-                                  expires_at: Time.at(omniauth['credentials']['expires_at']),
58
-                                  options: {user_id: omniauth['extra']['accounts'][0]['id']})
59
-      end
60
-    else
61
-      false
62
-    end
45
+    update(expires_at: Time.now + data['expires_in'], token: data['access_token'], refresh_token: data['refresh_token'].presence || refresh_token)
63 46
   end
64 47
 
65
-  private
66 48
   def endpoint
67 49
     client_options = "OmniAuth::Strategies::#{OmniAuth::Utils.camelize(self.provider)}".constantize.default_options['client_options']
68 50
     URI.join(client_options['site'], client_options['token_url'])
69 51
   end
70 52
 
71
-  @@provider_to_env_map = {'37signals' => 'THIRTY_SEVEN_SIGNALS'}
72 53
   def provider_to_env
73
-    @@provider_to_env_map[self.provider].presence || self.provider.upcase
54
+    PROVIDER_TO_ENV_MAP[provider].presence || provider.upcase
55
+  end
56
+
57
+  def oauth_key
58
+    ENV["#{provider_to_env}_OAUTH_KEY"]
59
+  end
60
+
61
+  def oauth_secret
62
+    ENV["#{provider_to_env}_OAUTH_SECRET"]
63
+  end
64
+
65
+  def self.provider_specific_options(omniauth)
66
+    case omniauth['provider']
67
+      when '37signals'
68
+        { user_id: omniauth['extra']['accounts'][0]['id'] }
69
+      else
70
+        {}
71
+    end
72
+  end
73
+
74
+  def self.initialize_or_update_via_omniauth(omniauth)
75
+    find_or_initialize_by(provider: omniauth['provider'], name: omniauth['info']['nickname'] || omniauth['info']['name']).tap do |service|
76
+      service.assign_attributes token: omniauth['credentials']['token'],
77
+                                secret: omniauth['credentials']['secret'],
78
+                                refresh_token: omniauth['credentials']['refresh_token'],
79
+                                expires_at: omniauth['credentials']['expires_at'] && Time.at(omniauth['credentials']['expires_at']),
80
+                                options: provider_specific_options(omniauth)
81
+    end
74 82
   end
75 83
 end

+ 2 - 2
app/models/user.rb

@@ -27,10 +27,10 @@ class User < ActiveRecord::Base
27 27
   has_many :agents, -> { order("agents.created_at desc") }, :dependent => :destroy, :inverse_of => :user
28 28
   has_many :logs, :through => :agents, :class_name => "AgentLog"
29 29
   has_many :scenarios, :inverse_of => :user, :dependent => :destroy
30
-  has_many :services, -> { order("services.name")}, :dependent => :destroy
30
+  has_many :services, -> { by_name('asc') }, :dependent => :destroy
31 31
 
32 32
   def available_services
33
-    Service.where("user_id = ? or global = true", self.id).order("services.name desc")
33
+    Service.available_to_user(self).by_name
34 34
   end
35 35
 
36 36
   # Allow users to login via either email or username.

+ 1 - 1
app/views/agents/_form.html.erb

@@ -34,7 +34,7 @@
34 34
             <% if @agent.try(:oauthable?) %>
35 35
               <div class="form-group type-select">
36 36
                 <%= f.label :service %>
37
-                <%= f.select :service_id, options_for_select(@agent.valid_services(current_user).collect { |s| ["(#{s.provider}) #{s.name}", s.id]}, @agent.service_id),{}, class: 'form-control' %>
37
+                <%= f.select :service_id, options_for_select(@agent.valid_services_for(current_user).collect { |s| ["(#{s.provider}) #{s.name}", s.id]}, @agent.service_id), {}, class: 'form-control' %>
38 38
               </div>
39 39
             <% end %>
40 40
           </div>

+ 2 - 1
app/views/scenario_imports/_step_two.html.erb

@@ -120,12 +120,13 @@
120 120
           </div>
121 121
         <% end %>
122 122
       </div>
123
+
123 124
       <% if agent_diff.requires_service? %>
124 125
         <div class='row'>
125 126
           <div class='col-md-4'>
126 127
             <div class="form-group type-select">
127 128
               <%= label_tag "scenario_import[merges][#{index}][service_id]", 'Service' %>
128
-              <%= select_tag "scenario_import[merges][#{index}][service_id]", options_for_select(agent_diff.agent_instance.valid_services(current_user).collect { |s| ["(#{s.provider}) #{s.name}", s.id]}, agent_diff.service_id.try(:current)), class: 'form-control' %>
129
+              <%= select_tag "scenario_import[merges][#{index}][service_id]", options_for_select(agent_diff.agent_instance.valid_services_for(current_user).collect { |s| ["(#{s.provider}) #{s.name}", s.id]}, agent_diff.service_id.try(:current)), class: 'form-control' %>
129 130
             </div>
130 131
           </div>
131 132
         </div>

+ 3 - 3
app/views/services/index.html.erb

@@ -7,7 +7,7 @@
7 7
         </h2>
8 8
       </div>
9 9
       <p>
10
-        Before you can authenticate with a service, you need to set it up. Have a look at the
10
+        Before you can authenticate with a service, you need to set it up. Have a look at the Huginn
11 11
         <%= link_to 'wiki', 'https://github.com/cantino/huginn/wiki/Configuring-OAuth-applications', target: :_blank %>
12 12
         for guidance.
13 13
       </p>
@@ -33,9 +33,9 @@
33 33
             <td>
34 34
               <div class="btn-group btn-group-xs">
35 35
                 <% if service.global %>
36
-                  <%= link_to 'Make private', toggle_availability_service_path(service), method: :post, data: { confirm: 'Are you sure you want to remove the access to this service for every user?'}, class: "btn btn-default" %>
36
+                  <%= link_to 'Make private', toggle_availability_service_path(service), method: :post, data: { confirm: 'Are you sure you want to remove access to your data on this service for other users?'}, class: "btn btn-default" %>
37 37
                 <% else %>
38
-                   <%= link_to 'Make global', toggle_availability_service_path(service), method: :post, data: { confirm: 'Are you sure you want to grant every user access to this service?'}, class: "btn btn-default" %>
38
+                   <%= link_to 'Make global', toggle_availability_service_path(service), method: :post, data: { confirm: 'Are you sure you want to grant every user on this system access to your data on this service?'}, class: "btn btn-default" %>
39 39
                 <% end %>
40 40
                 <%= link_to 'Delete', service_path(service), method: :delete, data: { confirm: 'Are you sure?' }, class: "btn btn-default btn-danger" %>
41 41
               </div>

+ 1 - 1
db/migrate/20140525150140_migrate_agents_to_service_authentication.rb

@@ -40,7 +40,7 @@ class MigrateAgentsToServiceAuthentication < ActiveRecord::Migration
40 40
     if Agent.where(type: ['Agents::BasecampAgent']).count > 0
41 41
       puts <<-EOF.strip_heredoc
42 42
 
43
-        Your Basecamp agents can not be migrated automatically. You need to manually register an application with 37signals and authenticate huginn to use it. 
43
+        Your Basecamp agents can not be migrated automatically. You need to manually register an application with 37signals and authenticate Huginn to use it.
44 44
         Have a look at the wiki (https://github.com/cantino/huginn/wiki/Configuring-OAuth-applications) if you need help.
45 45
 
46 46
 

+ 4 - 3
spec/controllers/services_controller_spec.rb

@@ -41,17 +41,18 @@ describe ServicesController do
41 41
   end
42 42
 
43 43
   describe "accepting a callback url" do
44
-    it "should update the users credentials" do
44
+    it "should update the user's credentials" do
45 45
       expect {
46 46
         get :callback, provider: 'twitter'
47 47
       }.to change { users(:bob).services.count }.by(1)
48 48
     end
49 49
 
50
-    it "should not work with an unknown provider" do
50
+    it "should work with an unknown provider (for now)" do
51 51
       request.env["omniauth.auth"]['provider'] = 'unknown'
52 52
       expect {
53 53
         get :callback, provider: 'unknown'
54
-      }.to change { users(:bob).services.count }.by(0)
54
+      }.to change { users(:bob).services.count }.by(1)
55
+      users(:bob).services.first.provider.should == 'unknown'
55 56
     end
56 57
   end
57 58
 end

+ 3 - 3
spec/models/concerns/oauthable.rb

@@ -16,14 +16,14 @@ shared_examples_for Oauthable do
16 16
     @agent.oauthable?.should == true
17 17
   end
18 18
 
19
-  describe "valid_services" do
19
+  describe "valid_services_for" do
20 20
     it "should return all available services without specifying valid_oauth_providers" do
21 21
       @agent = Agents::OauthableTestAgent.new
22
-      @agent.valid_services(users(:bob)).collect(&:id).sort.should == [services(:generic), services(:global)].collect(&:id).sort
22
+      @agent.valid_services_for(users(:bob)).collect(&:id).sort.should == [services(:generic), services(:global)].collect(&:id).sort
23 23
     end
24 24
 
25 25
     it "should filter the services based on the agent defaults" do
26
-      @agent.valid_services(users(:bob)).to_a.should == Service.where(provider: @agent.valid_oauth_providers)
26
+      @agent.valid_services_for(users(:bob)).to_a.should == Service.where(provider: @agent.valid_oauth_providers)
27 27
     end
28 28
   end
29 29
 end